feat(rds): default DatabaseCluser and DatabaseInstance `storageEncrypted` to `true` (under feature flag) #32695

blimmer · 2024-12-31T04:45:41Z

Issue # (if applicable)

Reason for this change

Today, if you don't remember to pass storageEncrypted: true to your DatabaseInstance or DatabaseCluster, your database's storage will be unencrypted.

In almost all cases, your database storage should be encrypted. Here are reasons I think we should use AWS-managed keys by default:

AWS Well-Architected security pillar says all data should be encrypted at rest.
There is no additional fee to use the AWS-managed KMS key
When creating RDS instances and clusters via the AWS Console UI, encryption with AWS-managed keys is enabled by default.
Adding encryption to a previously unencrypted database is an involved process that requires recreating your database from a snapshot. Applying encryption to an unencrypted database is time-consuming and a bad developer and user experience.
Other L2 constructs apply encryption by default, including Neptune and DocDb
If you don't encrypt your storage, you'll see warnings in the RDS UI telling you that your storage is unencrypted.
Users can easily retain the existing behavior.

Description of changes

This PR introduces a new feature flag, @aws-cdk/aws-rds:enableEncryptionAtRestByDefault. When set to true, storageEncrypted will default to true instead of undefined (which means "false").

To provide backward compatibility, this PR also introduces a new property, storageEncryptedLegacyDefaultValue. This allows leaving the StorageEncrypted property unset in the CloudFormation template, to avoid replacing Database instances/clusters.

Describe any new or updated permissions being added

N/A

Description of how you validated changes

Unit and integration tests.

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

… storageEncrypted

blimmer · 2024-12-31T04:47:07Z

...ges/@aws-cdk-testing/framework-integ/test/aws-appsync/test/integ.graphql-rds-serverlessv2.ts

@@ -35,6 +35,8 @@ class TestStack extends cdk.Stack {
      vpc,


This change affected a lot of integration tests. I opted to use the legacy behaviors for these tests because re-running the creation of all these databases would be extremely slow and expensive.

If someone at AWS with a "free"/"expensed" account would like to rerun all these failing tests, we could go that route, too.

I added explicit new tests for this behavior.

I think manually adding "StorageEncrypted": true to the templates would make those tests pass without the need to deploy the related resources (we expect the flag to work given that you added the new test).

In the past, I've received PR feedback that you should never manually update snapshot files. I just confirmed that I could add this property manually to the snapshots and that would be an alternative solution to updating the stack definitions.

I'd like a +1 from an approver that this would be OK before I spend the time doing it, based on previous feedback that snapshots should never be manually edited.

I've received PR feedback that you should never manually update snapshot files

👍 Sounds good!

About the isStorageLegacyUnencrypted comment.
Thanks for clarifying, I'm not sure why CFN defaults to unencrypted clusters and instances.
However, I left some comments on the implementation for small adjustments.

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

codecov · 2024-12-31T17:30:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.52%. Comparing base (bbdd42c) to head (14795d0).

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #32695   +/-   ##
=======================================
  Coverage   81.52%   81.52%           
=======================================
  Files         222      222           
  Lines       13715    13715           
  Branches     2417     2417           
=======================================
  Hits        11181    11181           
  Misses       2254     2254           
  Partials      280      280

Flag	Coverage Δ
suite.unit	`81.52% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`80.97% <ø> (ø)`
packages/aws-cdk-lib/core	`82.09% <ø> (ø)`

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

lpizzinidev

To provide backward compatibility, this PR also introduces a new property, isStorageLegacyUnencrypted. This allows leaving the StorageEncrypted property unset in the CloudFormation template, to avoid replacing Database instances/clusters.

Shouldn't the default behavior be applied when @aws-cdk/aws-rds:enableEncryptionAtRestByDefault: false? 🤔
The extra variable seems to add unnecessary complexity here.

lpizzinidev · 2025-01-03T15:38:41Z

...ges/@aws-cdk-testing/framework-integ/test/aws-appsync/test/integ.graphql-rds-serverlessv2.ts

@@ -35,6 +35,8 @@ class TestStack extends cdk.Stack {
      vpc,


I think manually adding "StorageEncrypted": true to the templates would make those tests pass without the need to deploy the related resources (we expect the flag to work given that you added the new test).

blimmer · 2025-01-03T20:16:58Z

@lpizzinidev - thanks for the review. I commented on your inline comment, and below is a comment that you requested changed for (it's annoying that GitHub won't let you respond inline to those).

Suggestion to reuse feature flag vs `isStorageLegacyUnencrypted`

This would work for v2, but if the feature flag is ever removed (it looks like all v1 flags were removed from v2 - see docs), this would no longer work. My understanding is that when v3 is released, the feature flag will be removed, and we will need to remember to re-add something like isStorageLegacyUnencrypted.

This is very similar to the isFromLegacyInstanceProps provided for a similar reason, to prevent replacing database resources (docs).

Unfortunately, because the behavior is to pass undefined today, it makes it a bit challenging to work with. CloudFormation reports that changing undefined -> false causes resource replacement. I feel fairly strongly that the new behavior should be to always explicitly set the value to true or false, which led me to this solution.

If there's another way I should be thinking about this (like retaining that feature flag indefinitely - I don't know if that's an option), let me know.

lpizzinidev · 2025-01-04T16:09:20Z

...ges/@aws-cdk-testing/framework-integ/test/aws-appsync/test/integ.graphql-rds-serverlessv2.ts

@@ -35,6 +35,8 @@ class TestStack extends cdk.Stack {
      vpc,


I've received PR feedback that you should never manually update snapshot files

👍 Sounds good!

About the isStorageLegacyUnencrypted comment.
Thanks for clarifying, I'm not sure why CFN defaults to unencrypted clusters and instances.
However, I left some comments on the implementation for small adjustments.

packages/aws-cdk-lib/aws-rds/lib/instance.ts

packages/aws-cdk-lib/aws-rds/README.md

…ryption-by-default

…Value

blimmer · 2025-01-04T18:14:43Z

@lpizzinidev - thanks for the thorough review and great comments. This is ready for re-review.

lpizzinidev

Thanks 🚀 Left comments for some final adjustments

packages/aws-cdk-lib/aws-rds/lib/private/util.ts

packages/aws-cdk-lib/aws-rds/README.md

…ryption-by-default

blimmer · 2025-01-06T21:15:38Z

@lpizzinidev - thanks! should be good to go.

lpizzinidev

Thanks 👍

…ryption-by-default

aws-cdk-automation · 2025-01-08T04:50:05Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 14795d0
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

blimmer added 7 commits December 30, 2024 19:49

chore: add feature flag

d80befc

feat: enable cluster encryption by default under feature flag

ff3521d

fix: add isStorageLegacyUnencrypted to work around false/undefined on…

319e467

… storageEncrypted

fix: existing integration tests

83d402d

docs: add readme note about encryption

9ce2d46

feat: enable encryption by default for db instances

221b872

fix: existing integration tests

75192b0

blimmer marked this pull request as ready for review December 31, 2024 04:45

github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels Dec 31, 2024

aws-cdk-automation requested a review from a team December 31, 2024 04:45

github-actions bot added the admired-contributor [Pilot] contributed between 13-24 PRs to the CDK label Dec 31, 2024

blimmer commented Dec 31, 2024

View reviewed changes

aws-cdk-automation previously requested changes Dec 31, 2024

View reviewed changes

fix: add default properties

04fe2b2

blimmer added 3 commits December 31, 2024 10:47

feat: add cluster integ test

d1c6c06

fix: code sample in readme

576289e

feat: add instance integ test

6bd263d

blimmer and others added 3 commits December 31, 2024 11:16

chore: add tests for thrown error

da306c0

fix: sync snapshot

6077185

Merge branch 'main' into enable-rds-encryption-by-default

4497f62

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 31, 2024

moltar approved these changes Jan 1, 2025

View reviewed changes

github-actions bot mentioned this pull request Jan 1, 2025

Monthly PR metrics report #32703

Open

lpizzinidev suggested changes Jan 3, 2025

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jan 3, 2025

lpizzinidev suggested changes Jan 4, 2025

View reviewed changes

blimmer added 6 commits January 4, 2025 10:56

Merge branch 'main' of github.com:blimmer/aws-cdk into enable-rds-enc…

c089ab2

…ryption-by-default

refactor: isStorageLegacyUnencrypted -> storageEncryptedLegacyDefault…

dbea499

…Value

fix: merge conflict issue with feature flag markdown

4af1515

refactor: common getStorageEncryptedProperty implementation

3c317b0

fix: again

26a9325

docs: rework docs

82f8e77

blimmer requested a review from lpizzinidev January 4, 2025 18:14

lpizzinidev suggested changes Jan 5, 2025

View reviewed changes

packages/aws-cdk-lib/aws-rds/lib/private/util.ts Outdated Show resolved Hide resolved

packages/aws-cdk-lib/aws-rds/README.md Show resolved Hide resolved

blimmer added 3 commits January 6, 2025 14:11

Merge branch 'main' of github.com:blimmer/aws-cdk into enable-rds-enc…

9130e4e

…ryption-by-default

fix: union type

1086ef1

docs: readme update

a327f10

blimmer requested a review from lpizzinidev January 6, 2025 21:15

fix: method signature

becf0a7

lpizzinidev approved these changes Jan 7, 2025

View reviewed changes

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 7, 2025

Merge branch 'main' of github.com:blimmer/aws-cdk into enable-rds-enc…

14795d0

…ryption-by-default

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rds): default DatabaseCluser and DatabaseInstance `storageEncrypted` to `true` (under feature flag) #32695

feat(rds): default DatabaseCluser and DatabaseInstance `storageEncrypted` to `true` (under feature flag) #32695

blimmer commented Dec 31, 2024 •

edited

Loading

blimmer Dec 31, 2024 •

edited

Loading

lpizzinidev Jan 3, 2025

blimmer Jan 3, 2025

lpizzinidev Jan 4, 2025 •

edited

Loading

aws-cdk-automation left a comment

codecov bot commented Dec 31, 2024 •

edited

Loading

lpizzinidev left a comment

lpizzinidev Jan 3, 2025

blimmer commented Jan 3, 2025 •

edited

Loading

lpizzinidev Jan 4, 2025 •

edited

Loading

blimmer commented Jan 4, 2025

lpizzinidev left a comment

blimmer commented Jan 6, 2025

lpizzinidev left a comment

aws-cdk-automation commented Jan 8, 2025

feat(rds): default DatabaseCluser and DatabaseInstance storageEncrypted to true (under feature flag) #32695

Are you sure you want to change the base?

feat(rds): default DatabaseCluser and DatabaseInstance storageEncrypted to true (under feature flag) #32695

Conversation

blimmer commented Dec 31, 2024 • edited Loading

Issue # (if applicable)

Reason for this change

Description of changes

Describe any new or updated permissions being added

Description of how you validated changes

Checklist

blimmer Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

lpizzinidev Jan 3, 2025

Choose a reason for hiding this comment

blimmer Jan 3, 2025

Choose a reason for hiding this comment

lpizzinidev Jan 4, 2025 • edited Loading

Choose a reason for hiding this comment

aws-cdk-automation left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 31, 2024 • edited Loading

Codecov Report

lpizzinidev left a comment

Choose a reason for hiding this comment

lpizzinidev Jan 3, 2025

Choose a reason for hiding this comment

blimmer commented Jan 3, 2025 • edited Loading

Suggestion to reuse feature flag vs isStorageLegacyUnencrypted

lpizzinidev Jan 4, 2025 • edited Loading

Choose a reason for hiding this comment

blimmer commented Jan 4, 2025

lpizzinidev left a comment

Choose a reason for hiding this comment

blimmer commented Jan 6, 2025

lpizzinidev left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Jan 8, 2025

AWS CodeBuild CI Report

feat(rds): default DatabaseCluser and DatabaseInstance `storageEncrypted` to `true` (under feature flag) #32695

feat(rds): default DatabaseCluser and DatabaseInstance `storageEncrypted` to `true` (under feature flag) #32695

blimmer commented Dec 31, 2024 •

edited

Loading

blimmer Dec 31, 2024 •

edited

Loading

lpizzinidev Jan 4, 2025 •

edited

Loading

codecov bot commented Dec 31, 2024 •

edited

Loading

blimmer commented Jan 3, 2025 •

edited

Loading

Suggestion to reuse feature flag vs `isStorageLegacyUnencrypted`

lpizzinidev Jan 4, 2025 •

edited

Loading